PR #9 Review — feat(vrt): add --config flag to qc vrt

Scope note: The title is narrow but the diff is broad — it touches CI workflows, package.json/lockfile, tsconfig, command files (app, backup, crawler, env, vrt), utils (api, config), login, and tests. All areas are covered below.

Blockers

1. package.json:1 version downgraded — will break npm publish

- "version": "0.4.3",   ← origin/main (base)
+ "version": "0.4.0",   ← HEAD

The base branch is at 0.4.3; this PR regresses it to 0.4.0. Any tag-triggered publish will push a lower version that npm will reject as already-published (or silently shadow a prior release). Looks like a rebase artefact — restore to 0.4.3 (or bump to 0.4.4/0.5.0 as appropriate).

2. .github/workflows/publish.yml — OIDC trusted publishing removed, replaced with long-lived static token

base publish.yml:7-9:

permissions:
  id-token: write  # required for npm OIDC trusted publishing

On HEAD the permissions block is gone entirely, and the publish step now uses NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}.

OIDC provenance tokens are scoped to a single workflow run and cannot be exfiltrated; a static NPM_TOKEN is a long-lived credential that, if leaked from the secrets store, grants permanent publish access. Restore the permissions: id-token: write block and the --provenance publish flag.

3. __tests__/utils/api.test.ts — 395 lines of behavioural tests deleted (524 → 129 lines)

Base: 524 lines covering getApplications, getEnvironments, getMetrics, getLogs, getProjects, getCrawlers, getBackups, the constructor, and 401/403/404 friendly-message paths. HEAD: 129 lines of structural stubs of the form expect(typeof client.getX).toBe('function').

The inline comment says these are "better suited as E2E tests", but the deleted tests mocked the SDK successfully and exercised error-handling branches E2E tests typically don't cover. Deleting them without replacement leaves the error-handling paths untested. Restore the behavioural tests with updated mocks (matching the refactored constructor), or add equivalent coverage before merging.

Warnings

W1. package.json:13 — dev script changed from tsx to ts-node --esm

- "dev": "tsx src/index.ts",          ← origin/main
+ "dev": "ts-node --esm src/index.ts", ← HEAD

ts-node is present in HEAD devDependencies (^10.9.1), so npm run dev will still run. However tsx (also in devDeps) handles ESM/CJS interop more reliably for modern TS projects. The regression looks unintentional — confirm it is deliberate.

W2. src/commands/vrt.ts:524-526 — parseBasicAuth returns undefined password for token-only input

function parseBasicAuth(auth: string): { username: string; password: string } {
  const [username, password] = auth.split(':');
  return { username, password };
}

--quant-auth mytoken (no colon) yields password === undefined at runtime despite the declared string type; Playwright httpCredentials then receives { username: 'mytoken', password: undefined }. Add a guard:

if (!auth.includes(':')) throw new Error('--quant-auth / --remote-auth must be in user:pass format');

W3. src/commands/vrt.ts:111-113 — parseFloat/parseInt with no NaN guard

const threshold = parseFloat(options.threshold || config.threshold?.toString() || '0.01');
const maxPages  = parseInt(options.maxPages  || ..., 10);
const maxDepth  = parseInt(options.maxDepth  || ..., 10);

parseFloat('abc') → NaN and downstream comparisons silently misbehave. Add Number.isNaN checks and surface a clear error.

W4. publish.yml / test.yml — action versions downgraded v6 → v4
Base uses actions/checkout@v6 and actions/setup-node@v6 (Node 24); HEAD downgrades both to @v4 (Node 18). v6 of these actions does exist, so this is a deliberate-looking downgrade, not a typo fix. Confirm the Node 24 → 18 change matches the project's supported engine range. Regardless of version, SHA-pinning the actions is recommended for supply-chain hardening.

W5. src/utils/config.ts:239 — parameter path shadows the path module import

// config.ts:3
import { join, resolve } from 'path';
// config.ts:239
export async function loadVRTConfigFrom(path: string): Promise<VRTConfig | null> {

The parameter shadows the module import — a readability hazard that will trip no-shadow lint rules. Rename to filePath. (Note: relative-path resolution is fine — resolveVRTConfig already calls resolve(cwd, configPath) before invoking this function.)

W6. src/commands/env.ts:700 — as any cast for SDK type workaround

// SDK incorrectly types this as void, but it actually returns data
const data = response.data as any;

The comment is honest, but as any disables downstream type checking. Prefer a narrower assertion (as { body?: unknown }) and link the upstream SDK issue so the workaround can be removed later.

Test coverage gaps

• Behavioural ApiClient error-handling tests deleted without replacement (see Blocker 3).
• parseBasicAuth has no tests — add cases for no colon, multiple colons (password containing :), empty string.
• No integration test exercising handleVRT with options.config set (the actual --config CLI path).
• No tests asserting graceful rejection of non-numeric threshold/maxPages/maxDepth.

Nits (non-blocking)

• vrt.ts help text for --quant-auth / --remote-auth could state the user:pass format so users don't discover it at runtime. Note also that credentials passed as CLI flags appear in ps/shell history/CI logs — the config-file path is the safer place for them.
• tsconfig lib: ES2020 cleanup (dropping DOM) is a sensible tightening — no action needed.

Findings withdrawn during verification (for transparency)

• VRTConfig.projects is typed required (config.ts:21), so Object.keys(config.projects) cannot throw on a valid config — no bug.
• AppEnvironment/AppData interfaces are still present in HEAD app.ts — not removed.
• Relative --config paths are resolved correctly by resolveVRTConfig — no bug.

Verdict: REQUEST CHANGES

Three blockers must be resolved before merge: the version regression in package.json, the removal of OIDC trusted publishing in favour of a static token, and the deletion of 395 lines of behavioural API tests without replacement. The remaining items are warnings, coverage gaps, and nits.